[WIP] Sampling code extraction#147
[WIP] Sampling code extraction#147bantonsson wants to merge 3 commits intoproject/sampling-extractionfrom
Conversation
| pub struct TraceRootSamplingInfo { | ||
| mechanism: SamplingMechanism, | ||
| rate: f64, | ||
| rl_effective_rate: Option<i32>, |
There was a problem hiding this comment.
| rl_effective_rate: Option<i32>, | |
| rl_effective_rate: Option<f32>, |
? don't we need the rate limiter rate to be a float between 0 and 1?
| | mechanism::LOCAL_USER_TRACE_SAMPLING_RULE | ||
| | mechanism::REMOTE_USER_TRACE_SAMPLING_RULE | ||
| | mechanism::REMOTE_DYNAMIC_TRACE_SAMPLING_RULE | ||
| | mechanism::APPSEC |
There was a problem hiding this comment.
In Python we set appsec to AUTO_KEEP/AUTO_REJECT and not user, is USER_PAIR correct here for this?
There was a problem hiding this comment.
Good catch 👀
We ported the sampler from the python code, a bit before the change fro, user to auto was made in this commit DataDog/dd-trace-py#14043
bantonsson
force-pushed
the
ban/sampling-extraction-again
branch
2 times, most recently
from
e72fef1 to
7dd3ba1
Compare
bantonsson
force-pushed
the
ban/sampling-extraction-again
branch
2 times, most recently
from
775d117 to
d64aafa
Compare
bantonsson
force-pushed
the
ban/sampling-extraction-again
branch
from
d64aafa to
bdc122f
Compare
| self.trace_id | ||
| } | ||
|
|
||
| fn with_span_properties<S, T, F>(&self, s: &S, f: F) -> T |
There was a problem hiding this comment.
If I'm following correctly, this has to be a callback for lifetime reasons. The PreSampledSpan will only live as long as the guard is in scope. Unless we use something like ouroboros, we can't return the guard with the thing it's guarding. I assume that's overkill / bad idea? Should the caller acquire the lock instead and pass it to this function? I don't think it would be much of a performance issue?
My concern isn't so much the use of a callback, but rather that it seems to be exposing an OTEL implementation detail that may not exist for the DD impl and might be confusing?
I don't have a strong opinion, just wanted to call it out.
| /// This is used for mapping between different attribute naming conventions | ||
| /// (e.g., OpenTelemetry to Datadog). Returns `Some(alternate_key)` if a mapping exists, | ||
| /// or `None` if the attribute key has no alternate mapping. | ||
| fn get_alternate_key<'b>(&self, key: &'b str) -> Option<Cow<'b, str>>; |
There was a problem hiding this comment.
Should this live on the trait, since only the DD impl will need it? Would it be better if matches took in an optional function / closure that applied this logic?
ekump
left a comment
ekump
left a comment
There was a problem hiding this comment.
Can't speak to the sampler logic, but just a couple non-blocking questions about some API stuff.
4 participants
What does this PR do?
This PR currently hides all the OTel specific things in the sampling code behind traits, so the sampling logic can be moved to a separate crate.
Motivation
Make the sampling code more generic and move it into a separate crate so it can be reused.
Additional Notes
The benchmarks are on par with the original code. Some improvements and a few minor regressions. There will be additional changes that move the code out.